toolchain: precheck passes under the pre-commit hook from a worktree - #1807
toolchain: precheck passes under the pre-commit hook from a worktree#1807sbryngelson wants to merge 2 commits into
Conversation
…eck passes from worktrees
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Fixes failures when running the coverage toolchain under a git hook launched from a worktree by ensuring git subprocesses ignore inherited GIT_* hook environment variables and operate on the intended repository directory.
Changes:
- Centralized “scrub
GIT_*from environment” logic intomfc.test.coverageand applied it to the shared git runner. - Updated
check_coverage_map_health.pyto route its git invocations through the centralized helper. - Removed the duplicated env-scrub helper from
test_coverage_unit.pyand imported the shared one instead.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
toolchain/mfc/test/test_coverage_unit.py |
Removes local _env_without_git helper and imports the shared implementation from mfc.test.coverage. |
toolchain/mfc/test/coverage.py |
Introduces _env_without_git() and applies it in _git() so git calls don’t inherit hook-provided GIT_* variables. |
.github/scripts/check_coverage_map_health.py |
Switches raw subprocess.run(["git", ...]) calls to use the shared _git() helper. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| sys.path.insert(0, str(Path(__file__).resolve().parents[2] / "toolchain")) | ||
| from mfc.test.coverage import COVERAGE_MAP_PATH, load_map, map_health # noqa: E402 | ||
| from mfc.test.coverage import COVERAGE_MAP_PATH, _git, load_map, map_health # noqa: E402 |
| # Git exports GIT_DIR and GIT_INDEX_FILE to hooks, and neither cwd nor `git -C` overrides them: | ||
| # under the pre-commit hook every call below would otherwise act on the committing repository. | ||
| return {k: v for k, v in os.environ.items() if not k.startswith("GIT_")} |
| # Git exports GIT_DIR and GIT_INDEX_FILE to hooks, and neither cwd nor `git -C` overrides them: | ||
| # under the pre-commit hook every call below would otherwise act on the committing repository. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1807 +/- ##
=======================================
Coverage 62.26% 62.26%
=======================================
Files 84 84
Lines 21558 21558
Branches 3188 3188
=======================================
Hits 13423 13423
Misses 5937 5937
Partials 2198 2198 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Committing from a git worktree fails the pre-commit hook: git exports
GIT_DIRandGIT_INDEX_FILEto hooks, and neithercwdnorgit -Coverrides them, so the throwaway repositories intest_coverage_unit.pyend up querying the committing repository instead. Two tests fail (test_verified_after_last_change_*) and the commit is blocked, leaving--no-verifyas the only way through.The test file already scrubbed
GIT_*for its own setup calls. This moves that scrub intocoverage._gitand routes the three raw git calls incheck_coverage_map_health.pythrough it, so every git subprocess the coverage machinery makes targets the directory it was given.Verified by making this commit through the hook from a worktree: precheck passes, the two tests pass under the hook's environment, and the full toolchain suite is 583 passed.